Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Backport 2.28] Add bignum test case generation script #6307

Merged

Conversation

wernerlewis
Copy link
Contributor

Description

Backport of #6093

Commits mostly the same in both PRs, a few differences from the PR on development:

  • Changes affecting test generation in CMake/make/windows are not included, as these test generation targets are not present in 2.28.
  • The --directory and --cmake-list arguments are not included in test_generation.py, as these are not required due to the above, and were not present in generate_psa_tests.py prior to this PR.
  • test_suite_mpi.generated.data is included in this PR.

Status

READY

Adds python script for generation of bignum test cases, with initial
classes for mpi_cmp_mpi test cases. Build scripts are updated to
generate test data.

Signed-off-by: Werner Lewis <[email protected]>
Signed-off-by: Werner Lewis <[email protected]>
Signed-off-by: Werner Lewis <[email protected]>
Clarification is added to docstrings, mostly in abstract classes.

Signed-off-by: Werner Lewis <[email protected]>
Previous implementation mixed the test case generation and the
recursive generation calls together. A separate method is added to
generate test cases for the current class' test function. This reduces
the need to override generate_tests().

Signed-off-by: Werner Lewis <[email protected]>
Signed-off-by: Werner Lewis <[email protected]>
Enforces fixed-width tuple types where mypy does not recognize.

Signed-off-by: Werner Lewis <[email protected]>
Version of pylint used in CI does not recognize abstract subclasses of
BaseTarget, so disable warning in these abstract classes.

Signed-off-by: Werner Lewis <[email protected]>
Signed-off-by: Werner Lewis <[email protected]>
Signed-off-by: Werner Lewis <[email protected]>
Signed-off-by: Werner Lewis <[email protected]>
When generating combinations of values, `itertools.combinations` will
not allow inputs to be repeated. This is replaced so that cases where
input values match are generated, i.e. ("0", "0").

Signed-off-by: Werner Lewis <[email protected]>
BaseTarget-derived targets are now added to TestGenerator.targets in
initialization. This reduces repeated code in generate_xxx_tests.py
scripts which use this framework.

Signed-off-by: Werner Lewis <[email protected]>
Signed-off-by: Werner Lewis <[email protected]>
Signed-off-by: Werner Lewis <[email protected]>
Wrapper function for itertools.combinations_with_replacement, with
explicit cast due to imprecise typing with older versions of mypy.

Signed-off-by: Werner Lewis <[email protected]>
Previous changes used the docstring of the test_generation module,
which does not inform a user about the script.

Signed-off-by: Werner Lewis <[email protected]>
@wernerlewis wernerlewis added needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Sep 21, 2022
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me as a backport of #6093. The differences are justified as not supporting --directory and --list-for-cmake in 2.28, plus committing the new generated file at the end.

I think we should actually reconcile the scripts as much as possible, because it's more work to keep certain features of test scripts out of the LTS than it is to just keep identical copies. But that's preexisting, and it's an area we need to revisit anyway, so we can handle it at that point.

Not labeling single-reviewer because I'd like a third pair of eyes to check that we aren't forgetting something extra that needs to be done in 2.28.

@gilles-peskine-arm gilles-peskine-arm added the priority-high High priority - will be reviewed soon label Oct 4, 2022
Copy link
Contributor

@minosgalanakis minosgalanakis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

I would argue that we could hardcode a directory using the default directive, and at least keep the option to the users of setting their own, without the smart bell and whisttles the cmake system provides, but it is not worth holding back the PR(especially since there is already an issue: 6295)

@gilles-peskine-arm gilles-peskine-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Oct 12, 2022
@gilles-peskine-arm gilles-peskine-arm merged commit 207b874 into Mbed-TLS:mbedtls-2.28 Oct 12, 2022
@wernerlewis wernerlewis deleted the bignum_test_script_2.28 branch October 12, 2022 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports priority-high High priority - will be reviewed soon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants